feat(developer): convert .keyman-touch-layout into KMX+ data and embed 🔱#15613
feat(developer): convert .keyman-touch-layout into KMX+ data and embed 🔱#15613mcdurdin wants to merge 1 commit intoepic/embed-osk-in-kmxfrom
Conversation
Basic conversion of .keyman-touch-layout into KMX+, handling merge with .kvks, structure, key caps, flicks, multitaps, and hints. TODO: 'special' key types, padding/gaps, conversion of *CAP* graphical key caps; search TODO-EMBED-OSK-IN-KMX for more Test-bot: skip
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
keymanapp-test-bot
bot
changed the title
github-actions
bot
added
developer/
common/
developer/compilers/
common/web/
feat
labels
| * | ||
| * .keyman-touch-layout JSON format definitions | ||
| * | ||
| * Follows scheams in /common/schemas/keyman-touch-layout/, using |
There was a problem hiding this comment.
| * Follows scheams in /common/schemas/keyman-touch-layout/, using | |
| * Follows schemas in /common/schemas/keyman-touch-layout/, using |
| // { "id": "K_F", "text": "blank", "sp": TouchLayout.TouchLayoutKeySp.blank, "width": 100, }, | ||
| // { "id": "K_G", "text": "spacer", "sp": TouchLayout.TouchLayoutKeySp.spacer, "width": 100, }, | ||
| // ]; | ||
|
|
There was a problem hiding this comment.
Do we need tests for loadTouchLayoutFile, especially for a non-existing, empty or invalid file?
| } | ||
|
|
||
| private addKeyFromTouchLayoutKey(kmxplus: KMXPlus.DependencySections, er: KMXPlus.LayrRow, idPrefix: string, key: TouchLayout.TouchLayoutKey): void { | ||
| const lk = new KMXPlus.KeysKeys(); |
There was a problem hiding this comment.
Can we use a better variable name than lk? keyskeys isn't ideal either but would at least have a connection to the type.
The same "problem" exists in other functions, but they are shorter so it's easier to see the definition of the variable.
| } | ||
|
|
||
| private addRowFromTouchLayoutRow(kmxplus: KMXPlus.KMXPlusData, entry: KMXPlus.LayrEntry, idPrefix: string, row: TouchLayout.TouchLayoutRow): void { | ||
| const er = new KMXPlus.LayrRow(); |
There was a problem hiding this comment.
| const er = new KMXPlus.LayrRow(); | |
| const layrRow = new KMXPlus.LayrRow(); |
| return kmxplus.strs.allocString(''); | ||
| } else if(text.trim() === '' && id.startsWith('U_')) { | ||
| // if key cap == U_xxxx[_yyyy], then we generate key cap from that | ||
| return kmxplus.strs.allocString(this.unicodeKeyIdToString(id)); |
There was a problem hiding this comment.
If id has an invalid U_ id this will result in an empty key cap and not produce a warning. I don't know if this can happen or if we know here that id always contains a valid U_ sequence. (Flagged by devin.ai)
| KMXPlus.DispItemFlags.keyCap123, //TODO-EMBED-OSK-IN-KMX | ||
| } | ||
| kmxplus.disp.disps.push(disp); | ||
| return kmxplus.strs.allocString(''); |
There was a problem hiding this comment.
Is this right that we return an empty string for *key* ?
| const lk = new KMXPlus.KeysKeys(); | ||
|
|
||
| lk.id = kmxplus.strs.allocString(idPrefix + key.id); | ||
| lk.to = this.getKeyCap(kmxplus, key.id, key.text); // kmxplus.strs.allocString(subKey.text); |
There was a problem hiding this comment.
devin.ai identified this problem - I can't tell, but it looks like it might be a real problem. We should probably add a unit test.
DispItem.toId references unprefixed key ID for main keys with special text patterns
When addKeyFromTouchLayoutKey processes a main key whose text matches the *...* pattern (e.g. *Enter*), the getKeyCap method creates a DispItem with toId set to the unprefixed key ID, while the key itself is stored with a prefixed ID.
Root Cause
At embed-osk-touch-layout.ts:99, the key's actual id is set to the prefixed form:
lk.id = kmxplus.strs.allocString(idPrefix + key.id); // e.g. "phone-default-K_ENTER"But at embed-osk-touch-layout.ts:100, getKeyCap is called with the unprefixed key.id:
lk.to = this.getKeyCap(kmxplus, key.id, key.text); // passes "K_ENTER"Inside getKeyCap at embed-osk-touch-layout.ts:208, when the text matches *...*, the DispItem.toId is allocated from the unprefixed id:
toId: kmxplus.strs.allocString(id), // allocates "K_ENTER" instead of "phone-default-K_ENTER"Contrast this with the hint DispItem at embed-osk-touch-layout.ts:141, which correctly uses the prefixed lk.id:
toId: lk.id, // correctly references "phone-default-K_Q"For subkeys (processed via keyFromSubKey), this is not a problem because subkey IDs are not prefixed — subKey.id matches lk.id. But for main keys, the mismatch means the runtime will fail to look up the display for these keys because the DispItem.toId won't match any key's actual ID.
Impact: Any main key (not subkey) with a special text pattern like *Enter*, *Shift*, etc. will have a broken display mapping in the compiled KMX+ output. The runtime won't find the DispItem for these keys, causing incorrect rendering of special key caps.
| lk.to = this.getKeyCap(kmxplus, key.id, key.text); // kmxplus.strs.allocString(subKey.text); | |
| lk.to = this.getKeyCap(kmxplus, idPrefix + key.id, key.text); // kmxplus.strs.allocString(subKey.text); |
| kmxplus.keys.keys.push(lk); | ||
| flicks.flicks.push(flick); | ||
| } | ||
|
|
There was a problem hiding this comment.
Another section that devin.ai flagged:
Subkey IDs are not deduplicated, risking duplicate KeysKeys entries
The keyFromSubKey method at embed-osk-touch-layout.ts:186-198 creates a new KeysKeys and pushes it unconditionally at lines 161 and 182. If the same subkey ID (e.g. K_1) is referenced by both a longpress (sk) and a flick on the same or different keys, duplicate KeysKeys entries with the same id will be added to kmxplus.keys.keys. Compare this with the LDML compiler in developer/src/kmc-ldml/src/compiler/keys.ts:314, which tracks used keys in a Set<string> to avoid duplicates. Depending on how the KMX+ builder serializes and the runtime looks up keys, this may cause issues or merely waste space. Worth investigating whether deduplication is needed here.
2 participants
Basic conversion of .keyman-touch-layout into KMX+, handling merge with .kvks, structure, key caps, flicks, multitaps, and hints.
TODO: 'special' key types, padding/gaps, conversion of CAP graphical key caps; search TODO-EMBED-OSK-IN-KMX for more
Test-bot: skip